🔥 feat: Auto-enforce Secure=true for Partitioned cookies in Cookie() #3976
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDocumentation updates to the Fiber context API showing proper cookie expiration using Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @darwin808, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the framework's cookie management capabilities by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ExpireCookie method to correctly expire cookies with specific Path or Domain attributes, addressing a limitation in the existing ClearCookie method. The implementation is a good start, but it's missing support for SameSite and Partitioned attributes, which are crucial for expiring modern cookies correctly. I've provided suggestions to add this functionality, update the corresponding documentation, and enhance the unit tests for better coverage and maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ctx_test.go (1)
4506-4562: Solid coverage for ExpireCookie; consider a couple of small assertionsThe test exercises the important combinations (Path, Domain, Path+Domain, Secure+HttpOnly) and verifies the Set-Cookie line in a robust, order‑independent way. Two optional tightenings you might consider:
- Also assert
require.Contains(t, setCookie, "expires=")in the Domain and Path+Domain (and possibly Secure+HttpOnly) cases, so every variant explicitly checks that the cookie is actually expired, not just that attributes are present.- Optionally add a single scenario that calls
c.ExpireCookie(...)instead ofc.Res().ExpireCookie(...)to exercise the Ctx helper as well as the Res method.Using
Response().Header.Peek(HeaderSetCookie)with string matching is consistent with the established pattern of manually inspecting Set-Cookie in Fiber tests, instead of relying onHeader.Cookie. Based on learnings, ...
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ctx_interface_gen.goctx_test.godocs/api/ctx.mdres.gores_interface_gen.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_interface_gen.gores.goctx_test.gores_interface_gen.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/api/ctx.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
ctx_interface_gen.gores.godocs/api/ctx.mdctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-01T03:33:22.283Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
🧬 Code graph analysis (2)
ctx_interface_gen.go (3)
res.go (1)
Cookie(104-116)client/request.go (1)
Cookie(778-778)bind_test.go (2)
Cookie(1512-1516)Cookie(1757-1761)
ctx_test.go (3)
res_interface_gen.go (1)
Res(13-174)res.go (1)
Cookie(104-116)constants.go (1)
HeaderSetCookie(197-197)
🔇 Additional comments (5)
res_interface_gen.go (1)
21-34: ClearCookie note and ExpireCookie API look consistent and clearThe new documentation accurately scopes ClearCookie’s limitations and positions ExpireCookie as the attribute-aware alternative; the ExpireCookie signature is minimal and consistent with the Cookie struct fields.
ctx_interface_gen.go (1)
327-340: Ctx-level ExpireCookie matches Res API and clarifies ClearCookie behaviorAdding ExpireCookie to Ctx with matching docs keeps the public surface coherent and clearly directs users away from ClearCookie for Domain/Path-specific cookies.
docs/api/ctx.md (2)
1743-1774: LGTM! Clear documentation of ClearCookie limitations.The updated caution note and reference to ExpireCookie provide excellent guidance for developers who need to clear cookies with specific Path or Domain attributes.
1775-1813: LGTM! Comprehensive and well-structured documentation.The ExpireCookie documentation is excellent:
- Clear method signature and description
- Practical examples covering common scenarios
- Important note about which Cookie fields are actually used
This will help developers properly expire cookies with Path/Domain attributes.
res.go (1)
202-219: LGTM! Documentation accurately reflects the limitation.The updated ClearCookie documentation correctly notes that it cannot clear cookies with specific Path or Domain attributes, and appropriately directs users to ExpireCookie for those cases.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3976 +/- ##
==========================================
+ Coverage 91.03% 91.24% +0.21%
==========================================
Files 119 119
Lines 10870 11050 +180
==========================================
+ Hits 9895 10083 +188
+ Misses 617 613 -4
+ Partials 358 354 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
darwin808
force-pushed
the
fix/expire-cookie-path-domain
branch
from
2a2f69c to
cf04f3f
Compare
|
The CI failure in The test expected <10 leaked goroutines but got 14, which can vary based on:
Could a maintainer please re-run CI? The |
|
@darwin808 Don't use force push, we can't see what's changing between commits. Makes reviewing harder. |
|
@darwin808 Tag me once the comments raised as addressed. Thanks 💪 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/api/ctx.md (1)
1806-1810: Consider using the typed constant in documentation for consistency.The example uses the string literal
"Strict"which works correctly due to case-insensitive matching. For documentation clarity, you might consider showing the constant usage alongside the string literal, since users might import and usefiber.CookieSameSiteStrictMode.// Using the constant: c.Res().ExpireCookie(&fiber.Cookie{ Name: "csrf", SameSite: fiber.CookieSameSiteStrictMode, })This is a minor suggestion for consistency with how the codebase defines these values.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ctx_interface_gen.goctx_test.godocs/api/ctx.mdres.gores_interface_gen.go
🚧 Files skipped from review as they are similar to previous changes (2)
- res_interface_gen.go
- ctx_test.go
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/api/ctx.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_interface_gen.gores.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.mdres.go
📚 Learning: 2025-07-19T14:06:29.884Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Applied to files:
docs/api/ctx.mdctx_interface_gen.gores.go
🧬 Code graph analysis (2)
ctx_interface_gen.go (2)
res.go (1)
Cookie(104-116)client/request.go (1)
Cookie(778-778)
res.go (3)
bind_test.go (2)
Cookie(1512-1516)Cookie(1757-1761)constants.go (3)
CookieSameSiteStrictMode(315-315)CookieSameSiteNoneMode(316-316)CookieSameSiteLaxMode(314-314)client/response.go (1)
Response(19-25)
🔇 Additional comments (4)
res.go (2)
204-206: LGTM!Clear documentation update that properly warns users about the ClearCookie limitation for cookies with Domain/Path attributes and directs them to use ExpireCookie.
221-265: Well-implemented cookie expiration with full attribute support.The implementation correctly handles:
- Path and Domain matching for proper cookie deletion per RFC 6265
- SameSite attribute with case-insensitive comparison
- Partitioned cookies (CHIPS)
- Auto-enforcement of
Secure=truewhenSameSite=NoneorPartitioned=trueper browser requirementsThe Acquire/Release pattern for fasthttp cookies is correctly used, and the deferred release ensures cleanup.
ctx_interface_gen.go (1)
329-340: LGTM!The interface definition correctly mirrors the implementation. The documentation properly notes all fields used (Name, Path, Domain, Secure, HTTPOnly, SameSite, Partitioned) and clarifies that Value/Expires are overwritten.
docs/api/ctx.md (1)
1773-1824: Comprehensive documentation for the new ExpireCookie API.The examples effectively demonstrate all supported use cases:
- Path-specific cookies
- Domain-specific cookies
- Security flags (Secure, HTTPOnly)
- SameSite attribute
- Partitioned cookies (CHIPS)
The note block clearly explains which fields are consumed and that Value/Expires are overwritten.
|
@gaby Addressed the review comments - added SameSite and Partitioned support, updated docs, and refactored tests to table-driven style. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctx_test.go (1)
4506-4598: Solid ExpireCookie coverage; consider releasing acquired contexts.The table‑driven test nicely exercises Path/Domain, SameSite variants, Secure/HTTPOnly, and Partitioned behavior with parallel subtests, which aligns well with the new API.
Minor nit: each subtest calls
app.AcquireCtx(&fasthttp.RequestCtx{})but never releases the context. For consistency with other tests and better pool hygiene, consider:Suggested cleanup
- app := New() - c := app.AcquireCtx(&fasthttp.RequestCtx{}) + app := New() + c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx_test.gores.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
res.goctx_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
📚 Learning: 2025-07-19T14:06:29.884Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Applied to files:
res.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
res.goctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-01T03:33:22.283Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
🧬 Code graph analysis (1)
ctx_test.go (3)
res.go (1)
Cookie(104-116)constants.go (3)
CookieSameSiteStrictMode(315-315)CookieSameSiteLaxMode(314-314)CookieSameSiteNoneMode(316-316)res_interface_gen.go (1)
Res(13-174)
🔇 Additional comments (1)
res.go (1)
202-207: ExpireCookie implementation and docs align with the intended semantics.The ClearCookie comment accurately documents its Path/Domain limitation, and
DefaultRes.ExpireCookiecorrectly:
- Constructs a deletion cookie (
""value,CookieExpireDelete).- Mirrors Path/Domain, when provided.
- Applies SameSite via case‑insensitive compare, and forces
SecureforSameSite=NoneandPartitionedcookies.- Preserves/overrides only the documented fields and emits the header via
SetCookie.This matches the new Res/Ctx interface contract and the RFC/best‑practice requirements around
SameSite=Noneand partitioned cookies. I don’t see further changes needed here.Also applies to: 221-267
|
@darwin808 can you check my hint |
There was a problem hiding this comment.
Pull request overview
This PR adds an ExpireCookie method to properly expire cookies that were set with specific Path or Domain attributes, addressing a limitation in the existing ClearCookie method which uses fasthttp.DelClientCookie that doesn't support these attributes.
Changes:
- Added
ExpireCookie(cookie *Cookie)method toDefaultResthat respects Path, Domain, and security attributes when expiring cookies - Updated
ClearCookiedocumentation to clarify its limitations with Path/Domain cookies - Added automatic
Secure=truehandling for Partitioned cookies in theCookiemethod to comply with CHIPS spec
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| res_interface_gen.go | Added ExpireCookie interface method with documentation noting Path/Domain support |
| ctx_interface_gen.go | Added ExpireCookie interface method to Ctx interface |
| res.go | Implemented ExpireCookie method with proper attribute handling and updated Cookie method for Partitioned cookies |
| ctx_test.go | Added comprehensive test cases for ExpireCookie covering path, domain, security flags, SameSite, and Partitioned attributes |
| docs/api/ctx.md | Added documentation and examples for ExpireCookie usage, added note to ClearCookie about its limitations |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @ctx_test.go:
- Around line 4548-4654: The subtests in Test_Ctx_ExpireCookie capture the loop
variable tc while running in parallel and also pass the table's Cookie value by
reference into ExpireCookie which mutates it; fix by shadowing the loop variable
(add "tc := tc" at the start of the t.Run closure) and pass a local copy of the
cookie to ExpireCookie (e.g., "cookieCopy := tc.cookie" then call
"c.Res().ExpireCookie(&cookieCopy)") so the shared table entry isn't mutated
across parallel subtests.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx_test.godocs/api/ctx.mdres.go
🚧 Files skipped from review as they are similar to previous changes (2)
- res.go
- docs/api/ctx.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-01T03:33:22.283Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-10-16T12:12:30.506Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in `ctx_test.go`, it is acceptable to use invalid CIDR notation such as `"0.0.0.1/31junk"` for testing purposes.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
🧬 Code graph analysis (1)
ctx_test.go (3)
res.go (1)
Cookie(104-116)constants.go (4)
CookieSameSiteStrictMode(315-315)CookieSameSiteLaxMode(314-314)CookieSameSiteNoneMode(316-316)CookieSameSiteDisabled(313-313)res_interface_gen.go (1)
Res(13-174)
🔇 Additional comments (1)
ctx_test.go (1)
1214-1215: Good clarification on Partitioned+Secure auto-fix.This comment helps explain why the Partitioned-invalid test case is gone and documents the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ctx_test.go (3)
2483-2513: Release acquired ctx in Test_Ctx_IP_ProxyHeader (avoid leaks across table loop).
Each loop iteration callsapp.AcquireCtx(...)but neverapp.ReleaseCtx(...).Proposed fix
for _, proxyHeaderName := range proxyHeaderNames { app := New(Config{ProxyHeader: proxyHeaderName}) c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) c.Request().Header.Set(proxyHeaderName, "0.0.0.1") require.Equal(t, "0.0.0.1", c.IP()) @@ c.Request().Header.Set(proxyHeaderName, "not-valid-ip") require.Equal(t, "not-valid-ip", c.IP()) }Note: if you keep the
deferinside the loop, it defers until the test ends (still correct, but less ideal). Prefert.Run(...)subtests withdefer/t.Cleanup, or explicitlyapp.ReleaseCtx(c)at loop end.
2516-2546: Release acquired ctx in Test_Ctx_IP_ProxyHeader_With_IP_Validation (avoid leaks).
Same issue as the non-validation variant.Proposed fix
for _, proxyHeaderName := range proxyHeaderNames { app := New(Config{EnableIPValidation: true, ProxyHeader: proxyHeaderName}) c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) // when proxy header & validation is enabled and the value is a valid IP, we return it c.Request().Header.Set(proxyHeaderName, "0.0.0.1") require.Equal(t, "0.0.0.1", c.IP()) @@ c.Request().Header.Set(proxyHeaderName, "not-valid-ip") require.Equal(t, "0.0.0.0", c.IP()) }Same note re:
deferin a loop;t.Run+t.Cleanupis usually cleaner here.
3386-3419: Test_Ctx_Scheme: addt.Parallel()+ release acquired ctx.
This test currently (a) doesn’t callt.Parallel()and (b) doesn’t release the acquired ctx.Proposed fix
func Test_Ctx_Scheme(t *testing.T) { + t.Parallel() app := New() freq := &fasthttp.RequestCtx{} freq.Request.Header.Set("X-Forwarded", "invalid") c := app.AcquireCtx(freq) + defer app.ReleaseCtx(c)As per coding guidelines, tests should call
t.Parallel()at the start.
♻️ Duplicate comments (1)
ctx_test.go (1)
2226-2340: Fix missing ctx release in Test_Ctx_IsProxyTrusted (pool leak / flake risk).
Second scenario (Lines 2239-2241) acquires a ctx but never releases it.Proposed fix
{ app := New(Config{ TrustProxy: false, }) c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) require.True(t, c.IsProxyTrusted()) }
🧹 Nitpick comments (1)
ctx_interface_gen.go (1)
315-318: Tighten IsProxyTrusted doc wording/grammar (avoid ambiguity).Consider rephrasing Line 316 to be grammatical and explicit (e.g., “If
Config.TrustProxyis false, it returns true.”) and add a trailing period.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx_interface_gen.goctx_test.godocs/api/ctx.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api/ctx.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_interface_gen.goctx_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (13)
📓 Common learnings
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
📚 Learning: 2024-10-16T14:00:50.724Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3170
File: ctx.go:1825-1826
Timestamp: 2024-10-16T14:00:50.724Z
Learning: In the Fiber framework, the `IsProxyTrusted()` function returns `true` when `TrustProxy` is `false`, maintaining compatibility with version 2 behavior.
Applied to files:
ctx_interface_gen.goctx_test.go
📚 Learning: 2025-07-19T14:06:29.884Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Applied to files:
ctx_interface_gen.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
ctx_test.go
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-07-02T13:29:56.992Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-01T03:33:22.283Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.go
🔇 Additional comments (1)
ctx_test.go (1)
2698-2720: Benchmark config change looks fine.
SettingProxyHeaderin the benchmark setup matches the new test configuration style.
Fixes gofiber#2878 ClearCookie() does not work for cookies that were set with specific Path or Domain attributes because browsers require matching attributes to properly clear a cookie. This adds ExpireCookie(cookie Cookie) which allows specifying the Path, Domain, Secure, and HTTPOnly attributes when expiring a cookie, ensuring the browser properly matches and clears it.
- Handle SameSite attribute (Strict/Lax/None) for proper cookie expiration - Handle Partitioned attribute for CHIPS cookies - Auto-set Secure=true when SameSite=None or Partitioned=true (per RFC) - Update documentation with new examples - Refactor tests to table-driven style with comprehensive coverage
- Add default case to SameSite switch statement (revive) - Reorder struct fields for better memory alignment (govet)
- Add defer app.ReleaseCtx(c) for proper context cleanup - Reorder struct fields for optimal memory alignment
- Add Secure enforcement for Partitioned cookies per CHIPS spec - Update test to reflect new auto-fix behavior
- Add CookieSameSiteDisabled handling in ExpireCookie to not set SameSite attribute when explicitly disabled - Fix documentation signature to show fiber.Res receiver instead of fiber.Ctx - Add test case for CookieSameSiteDisabled to verify SameSite is not present in Set-Cookie header
Remove ExpireCookie method - the existing Cookie() method handles this use case by setting Expires to fasthttp.CookieExpireDelete. Updated documentation to show the recommended approach. Fixes gofiber#2878
darwin808
force-pushed
the
fix/expire-cookie-path-domain
branch
from
2375d3e to
2e985d2
Compare
|
@darwin808 thx for the adjustments I checked and now only documentation adjustments are included. |
|
i mean this |
|
@darwin808 ping |
- Add Secure enforcement for Partitioned cookies per CHIPS spec - Update test to reflect new auto-fix behavior
darwin808
changed the title
darwin808
changed the title
|
@ReneWerner87 Done! Added the Partitioned → Secure=true auto-enforcement in 338dad1 and updated the PR title/description. Ready for review. |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
3 participants
Summary
Secure=truewhenPartitioned=trueis set on a cookie, per the CHIPS specificationfasthttp.CookieExpireDeleteconstantPath/DomainattributesChanges
Cookie enforcement (
res.go):Secure=truewhen a cookie hasPartitioned=true, as required by the CHIPSspec
Documentation (
docs/api/ctx.md):fasthttp.CookieExpireDeleteinstead of manual time calculation for cookie deletion examplesPath/DomainattributesTest plan
Fixes #2878